-
-
Notifications
You must be signed in to change notification settings - Fork 677
Remove sage-conf #40327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove sage-conf #40327
Conversation
It appears that at least in some configs sage_config is broken on 10.8.beta4, see |
Now that the dependencies are in, this would be ready to go. But there are a couple of CI issues, mostly due to missing gap packages. I have no idea how this works in sage-the-distro; probably I've implicitly removed the setting of certain env variables. @dimpase could you have a look? |
Documentation preview for this PR (built with commit 7b5923f; changes) is ready! 🎉 |
I don't see much specifically related to GAP packages in CI runs. |
There were issues like:
and
and
https://github.com/sagemath/sage/actions/runs/17903937351/job/50901861649#step:11:44 |
All of these work for me
is either a failure while installing GAP package, or the external GAP used has the package Ditto with |
Oh okay, so this is just the usual hiccup of the incremental CI runs. In fact, the non-incremental run shows no such errors as well: https://github.com/sagemath/sage/actions/runs/17962928730/job/51089863679?pr=40327 Thanks Dima for looking into this! Ready for review now. |
… and run long tests on it
Co-authored-by: user202729 <[email protected]>
the 5 files above still mention These must be cleaned up appropriately. |
This will be done by reverting the revert. I can do this as part of this PR (at the cost of bloating it even more), or we keep |
The sage env error is indeed still there:
so this needs a decision on how to resolve this doctest failure: see #40882 (comment) for more details and options (and thanks @user202729 for the suggestion to improve the doctest to issue more meaningful details) |
Copying my remark about the First and foremost, it tests that you can import Edit: If on the other hand SAGE_ROOT is meaningless then why is it in |
so, which variable was needed to be set?
I don't understand - it's possible to have sagelib needing something like It's certainly possible to test for this alone. If this is what you would like to see here,
|
by the way, does one need |
Yes, you do need these functions but not in the |
@dimpase @vbraun @orlitzky could we please get this in? (For the failing test-long, see #41002 - the build for a clean image succeeds: https://github.com/sagemath/sage/actions/runs/18301584388/job/52110502440?pr=40327#step:11:18703) |
test-long looks sane, but maybe you can just retrigger it just in case? since #41002 is [ci fix], it should pass this time. |
this branch has not got all the changes we had for |
Yes, I've not reinstated #40765 here to keep this PR as self-contained as possible. But I can do this here as well, if you prefer doing it that way. The |
I'd much prefer having a branch which has no obvious defects. |
I've now reverted the revert, except for tobiasdiez@72876f0 (because I kind of remember a discussion about this commit it another PR). Let's see if CI is happy. |
On October 9, 2025 10:51:57 PM CDT, Tobias Diez ***@***.***> wrote:
tobiasdiez left a comment (sagemath/sage#40327)
I've now reverted the revert, except for tobiasdiez@72876f0 (because I kind of remember a discussion about this commit it another PR).
this commit has been redone in a better way by Volker, so it can go.
…Let's see if CI is happy.
|
sagemathgh-40327: Remove sage-conf <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> No longer needed after sagemath#39030. Fixes sagemath#40793. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ ] The title is concise and informative. - [ ] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies - sagemath#39030 - sagemath#40357 - sagemath#40765 <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#40327 Reported by: Tobias Diez Reviewer(s): Dima Pasechnik
sagemathgh-40327: Remove sage-conf <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> No longer needed after sagemath#39030. Fixes sagemath#40793. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ ] The title is concise and informative. - [ ] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies - sagemath#39030 - sagemath#40357 - sagemath#40765 <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#40327 Reported by: Tobias Diez Reviewer(s): Dima Pasechnik
Darn, I was not paying attention, I was using that :) I am trying to wrap my head around the construct |
looks like there are ≥ 2 people wanting to remove it. By our dispute voting rule, if you can look for ≥ 4 people who would vote against, then you can propose a pull request reverting this. (be quick before more pull requests get merged on top of it making it more difficult to revert…) ( |
any revert of this sort of PR throws a huge spanner into the works. I am sure all the kinks can be ironed out without it. |
To be clear, I am not against. It just caught me unaware, and I had a technical question for my own edification. In the big picture, stuff that was never a very happy split was moved to a new location and some adjustments. The most important bits have now been moved to |
This is to support sage-the-distro. I am pleased to report that I have not had to think about this in a long time, but I think the issue is that, when |
I'm pretty sure this was not the last adjustment to the config-system. Actually, it's more a start - now that we only have to worry about meson writing this file, we can use it more productively and move magic code like the
Is this only because you manually set these variables in gentoo, or do you see any other advantages in such as split? My goal would be that in the near future, you can use the meson-generated file and don't have to set anything manually. I think I asked you this before, so please accept my apology for forgetting, but what modifications do you have to do for gentoo? Looking at https://github.com/cschwan/sage-on-gentoo/blob/master/sci-mathematics/sagemath/files/sage-conf.py-10.8, it appears to me that meson should generate a mostly correct config file once you set the correct prefix. |
I'd say so. If I am going to replace those variables, having them in one file where I only touch them and nothing else feels ideal.
And once I am finished with the conference I am at right now, trying to see what happens with pure meson without patching should be my first order of the day. the main point of the old system was you coud just define whatever you wanted to override and leave the rest alone. If anything, my feeling is that too many variables seem to need adjustment in that file in sage-the-distro. |
Sage-on-gentoo builds again. This is the current state of affair https://github.com/cschwan/sage-on-gentoo/blob/master/sci-mathematics/sagemath/files/sagemath-10.8-config.py.in.patch a lot of that stuff is self inflicted. One day we'll get rid of jmol for real. |
|
No longer needed after #39030. Fixes #40793.
📝 Checklist
⌛ Dependencies